BUG: Fix all-NaT when ArrowEA.astype to categorical #62055
Conversation
arthurlw
changed the title
arthurlw
changed the title
|
Using In the first example, the code does not think they are comparable in |
|
So we should start by fixing should_compare? |
Sorry I missed this earlier. The fix I pushed addresses that and resolves the second issue too. |
pandas/core/indexes/datetimes.py
Outdated
| # If we have tz, we can compare to tzaware | ||
| return isinstance(dtype, DatetimeTZDtype) | ||
|
|
||
| from pandas import ArrowDtype |
There was a problem hiding this comment.
can go at the top of the file
pandas/core/indexes/datetimes.py
Outdated
|
|
||
| return ( | ||
| pa.types.is_date32(dtype.pyarrow_dtype) | ||
| or pa.types.is_date64(dtype.pyarrow_dtype) |
There was a problem hiding this comment.
i think timestamp is comparable but date is not
There was a problem hiding this comment.
The original issue was with pyarrow date dtypes, which compare fine when using astype, so I think they should be treated as comparable here
There was a problem hiding this comment.
dti = pd.date_range("2016-01-01", periods=3)
item = dti[0].date()
>>> (item == dti)[0]
np.False_
We don't have a non-pyarrow date dtype, but if we did, it would not be considered comparable to datetime64
There was a problem hiding this comment.
I think in that case the question is whether we want astype with categoricals to succeed here, or whether astype between pyarrow date and datetime64 should be disallowed for consistency
There was a problem hiding this comment.
do we have analogous special-casing for the non-pyarrow dt64 that im missing?
There was a problem hiding this comment.
Then I expect it shouldn’t be necessary here. I’ll take a closer look on monday
There was a problem hiding this comment.
i think the relevant special-casing is in Index._maybe_downcast_for_indexing. Take a look for the inferred_type checks
| expected = array([0, 1, 2], dtype="Int64").astype("category") | ||
| tm.assert_extension_array_equal(result, expected) | ||
|
|
||
| def test_arrow_array_astype_to_categorical_dtype_temporal(self): |
There was a problem hiding this comment.
can you also test the intermediate steps that used to fail
| method = clean_reindex_fill_method(method) | ||
| orig_target = target | ||
| target = self._maybe_cast_listlike_indexer(target) | ||
| target, self = target._maybe_downcast_for_indexing(self) |
There was a problem hiding this comment.
does doing this here mean that we don't need to do it again below?
|
|
||
| if isinstance(res, ExtensionArray): | ||
| if res_index.dtype == "string[pyarrow]" and is_timedelta64_dtype( | ||
| self.dtype |
There was a problem hiding this comment.
in this file we usually just check self.dtype.kind == "m"
| res = keyarr | ||
| return Index(res, dtype=res.dtype) | ||
|
|
||
| res_index = Index(res, dtype=res.dtype) |
There was a problem hiding this comment.
should wrapping in an Index go just once at the end?
|
Hey @jbrockmendel, just to clarify, the deprecation in #62158 implies that date types aren’t comparable with datetime64 types. If that’s the case, then the first example in this issue is actually the expected behavior (i.e. it should return all NaTs). arr = pd.array(
["2017-01-01", "2018-01-01", "2019-01-01"],
dtype="date32[day][pyarrow]"
)
cats = pd.Index(['2017-01-01', '2018-01-01', '2019-01-01'], dtype="M8[s]")
dtype = pd.CategoricalDtype(cats, ordered=False)
arr.astype(cats.dtype) # <- works
arr.astype(dtype) # <- all-NaT |
|
The deprecation in #62158 means that the approach here won't quite work, you're right. But that deprecation won't affect the conversion of dates->datetime64 with astype, so i'd still expect date->categorical[datetime64] to work |
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
2 participants
Added type annotations to new arguments/methods/functions.doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.